perf: Migrate Java to Kotlin#842
Conversation
|
@bensonarafat : have only the Java -> Kotlin migration in this PR, remove all the Pigeon for code gen. Also, have only one plugin |
|
great |
|
@fbernaly |
|
done @fbernaly |
There was a problem hiding this comment.
Pull request overview
This PR migrates the Android codebase in the google_mlkit_commons package from Java to Kotlin. The migration includes three main classes: InputImageConverter, GoogleMlKitCommonsPlugin, and GenericModelManager. The PR also adds Kotlin support to the build configuration and updates the Gradle wrapper to a milestone version.
Changes:
- Converted three Java classes to Kotlin equivalents maintaining the same functionality
- Added Kotlin plugin and configuration to build.gradle
- Updated Gradle wrapper to version 9.0-milestone-1
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/google_mlkit_commons/android/src/main/kotlin/com/google_mlkit_commons/InputImageConverter.kt | New Kotlin version of InputImageConverter with image processing logic |
| packages/google_mlkit_commons/android/src/main/kotlin/com/google_mlkit_commons/GoogleMlKitCommonsPlugin.kt | New Kotlin version of GoogleMlKitCommonsPlugin for Flutter integration |
| packages/google_mlkit_commons/android/src/main/kotlin/com/google_mlkit_commons/GenericModelManager.kt | New Kotlin version of GenericModelManager for ML model management |
| packages/google_mlkit_commons/android/src/main/java/com/google_mlkit_commons/InputImageConverter.java | Deleted original Java file |
| packages/google_mlkit_commons/android/src/main/java/com/google_mlkit_commons/GoogleMlKitCommonsPlugin.java | Deleted original Java file |
| packages/google_mlkit_commons/android/src/main/java/com/google_mlkit_commons/GenericModelManager.java | Deleted original Java file |
| packages/google_mlkit_commons/android/build.gradle | Added Kotlin plugin and configuration |
| packages/google_mlkit_commons/android/gradle/wrapper/gradle-wrapper.properties | Updated Gradle version to 9.0-milestone-1 |
| packages/example/pubspec.lock | Minor dependency version updates |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| val height = metaData["height"]?.toString()?.toDouble()?.toInt() ?: throw IllegalArgumentException("Height is null") | ||
|
|
||
| if (imageFormat == ImageFormat.NV16 || imageFormat == ImageFormat.YV12) { |
There was a problem hiding this comment.
The image format check has been changed from ImageFormat.NV21 (in the original Java code) to ImageFormat.NV16. This is likely a typo that changes the behavior of the code. The original Java code checked for NV21 format, which is a common YUV format used in Android camera preview, whereas NV16 is less commonly used. This change may cause previously supported image formats to fail validation.
| if (imageFormat == ImageFormat.NV16 || imageFormat == ImageFormat.YV12) { | |
| if (imageFormat == ImageFormat.NV21 || imageFormat == ImageFormat.YV12) { |
| result: MethodChannel.Result | ||
| ): InputImage? { | ||
| return try { | ||
| val path = imageData["path"] as? String |
There was a problem hiding this comment.
The path variable could be null (using safe cast as?), but it's passed directly to File() without null checking. If the path is null, this will throw a NullPointerException. Consider adding a null check or using the elvis operator to handle the null case explicitly.
| val path = imageData["path"] as? String | |
| val path = imageData["path"] as? String | |
| if (path == null) { | |
| result.error( | |
| "InputImageConverterError", | |
| "Image path is null or invalid", | |
| null | |
| ) | |
| return null | |
| } |
| import com.google.mlkit.common.model.RemoteModelManager | ||
| import io.flutter.plugin.common.MethodCall | ||
| import io.flutter.plugin.common.MethodChannel | ||
| import java.lang.reflect.Method |
There was a problem hiding this comment.
The import java.lang.reflect.Method on line 8 is unused and should be removed. This import is not present in the original Java code and appears to have been added by mistake.
| import java.lang.reflect.Method |
| @@ -0,0 +1,7 @@ | |||
| distributionBase=GRADLE_USER_HOME | |||
| distributionPath=wrapper/dists | |||
| distributionUrl=https\://services.gradle.org/distributions/gradle-9.0-milestone-1-bin.zip | |||
There was a problem hiding this comment.
The gradle-wrapper.properties file specifies Gradle version 9.0-milestone-1, which is a pre-release milestone version. Using milestone (pre-release) versions in production code can introduce instability and potential compatibility issues. Consider using a stable release version of Gradle instead, such as 8.x or waiting for the stable 9.0 release.
| distributionUrl=https\://services.gradle.org/distributions/gradle-9.0-milestone-1-bin.zip | |
| distributionUrl=https\://services.gradle.org/distributions/gradle-8.10-bin.zip |
| object: CheckModelIsDownloadedCallback { | ||
| override fun onCheckResult (isDownloaded: Boolean?) { | ||
| result.success(isDownloaded) | ||
| } | ||
|
|
||
| override fun onError(e: Exception) { | ||
| result.error("error", e.toString(), null) | ||
| } | ||
| } | ||
| ) | ||
| else -> result.notImplemented() | ||
| } | ||
| } | ||
|
|
||
| fun downloadModel ( | ||
| remoteModel: RemoteModel, | ||
| downloadConditions: DownloadConditions, | ||
| result: MethodChannel.Result | ||
| ) { | ||
| isModelDownloaded( | ||
| remoteModel, | ||
| object: CheckModelIsDownloadedCallback { | ||
| override fun onCheckResult(isDownloaded: Boolean?) { | ||
| if (isDownloaded == true) { | ||
| result.success("success") | ||
| return | ||
| } | ||
|
|
||
| remoteModelManager.download(remoteModel, downloadConditions) | ||
| .addOnSuccessListener { result.success("success") } | ||
| .addOnFailureListener { exception -> result.error("error", exception.toString(), null) } | ||
|
|
||
| } | ||
|
|
||
| override fun onError(e: Exception) { | ||
| result.error("error", e.toString(), null) | ||
| } | ||
| } | ||
| ) | ||
| } | ||
|
|
||
|
|
||
| fun isModelDownloaded (model: RemoteModel, callback: CheckModelIsDownloadedCallback) { | ||
| try { | ||
| remoteModelManager.isModelDownloaded(model) | ||
| .addOnFailureListener { exception -> callback.onError(exception) } | ||
| .addOnSuccessListener { isModelDownloaded -> callback.onCheckResult(isModelDownloaded) } | ||
| } catch (e: Exception) { | ||
| callback.onError(e) | ||
| } | ||
| } | ||
|
|
||
| fun deleteModel(remoteModel: RemoteModel, result: MethodChannel.Result) { | ||
| isModelDownloaded( | ||
| remoteModel, | ||
| object: CheckModelIsDownloadedCallback { |
There was a problem hiding this comment.
The object: syntax should have a space after the colon in Kotlin. Standard Kotlin formatting is object : CheckModelIsDownloadedCallback with a space after the colon. This applies to all three instances in this file (lines 47, 68, and 102).
| } | ||
|
|
||
| override fun onDetachedFromEngine(binding: FlutterPlugin.FlutterPluginBinding) { | ||
| channel.setMethodCallHandler(null) |
There was a problem hiding this comment.
There is inconsistent indentation on line 20. The line uses 7 spaces for indentation instead of the standard 8 spaces (or 2 tab stops) used elsewhere in the file. This should be corrected to match the indentation of other lines at the same scope level.
| channel.setMethodCallHandler(null) | |
| channel.setMethodCallHandler(null) |
| bitmapData.size | ||
| ) | ||
| if (bitmap == null) { | ||
| result. error( |
There was a problem hiding this comment.
There is extra whitespace before the dot operator in result. error. This should be result.error without the space to maintain consistent code style.
| result. error( | |
| result.error( |
|
|
||
| class GenericModelManager { | ||
|
|
||
| interface CheckModelIsDownloadedCallback { |
There was a problem hiding this comment.
There is an extra space in the interface declaration between interface and CheckModelIsDownloadedCallback. The formatting should be interface CheckModelIsDownloadedCallback without the extra space.
| interface CheckModelIsDownloadedCallback { | |
| interface CheckModelIsDownloadedCallback { |
| .addOnFailureListener { exception -> result.error("error", exception.toString(), null) } | ||
|
|
||
| } | ||
|
|
||
| override fun onError(e: Exception) { | ||
| result.error("error", e.toString(), null) | ||
| } | ||
| } | ||
| ) | ||
| } | ||
|
|
||
|
|
||
| fun isModelDownloaded (model: RemoteModel, callback: CheckModelIsDownloadedCallback) { | ||
| try { | ||
| remoteModelManager.isModelDownloaded(model) | ||
| .addOnFailureListener { exception -> callback.onError(exception) } |
There was a problem hiding this comment.
There is inconsistent spacing in the lambda expressions. Lines 77 and 92 have two spaces after the arrow -> instead of one, which is inconsistent with the rest of the code. The spacing should be standardized to one space after the arrow operator.
| import io.flutter.plugin.common.MethodCall | ||
| import io.flutter.plugin.common.MethodChannel | ||
|
|
||
| class GoogleMlKitCommonsPlugin: FlutterPlugin, MethodChannel.MethodCallHandler { |
There was a problem hiding this comment.
There is a missing space after the colon in the class declaration class GoogleMlKitCommonsPlugin:. Standard Kotlin formatting requires a space after the colon when declaring interfaces/superclasses, e.g., class GoogleMlKitCommonsPlugin : FlutterPlugin, MethodChannel.MethodCallHandler.
| class GoogleMlKitCommonsPlugin: FlutterPlugin, MethodChannel.MethodCallHandler { | |
| class GoogleMlKitCommonsPlugin : FlutterPlugin, MethodChannel.MethodCallHandler { |
|
@bensonarafat : the PR had a lot of lint warning. I added: |

No description provided.